Use debugger highlighting for triggering frames to render (improves screenshots) #927
Conversation
Since the initial implementation of `FrameManager`, Electron added an API for Chromium's remote debugging protocol. One of the debugger's capabilities is to visually highlight a portion of the page. This change uses that functionality to force a new frame to be rendered. This is much more reliable than the way Nightmare currently tries to force new frames to render by fiddling around with the DOM (see issues segment-boneyard#555, segment-boneyard#736, segment-boneyard#809). It also has the benefit of not doing anything page content can observe, ensuring that any JS or CSS won't modify the page in response to Nightmare's attempt to take a screenshot. In future versions of the protocol, it will be possible to directly capture an image of the page, but that feature is still experimental (so it could be removed) and Electron does not yet support it anyhow. Something to keep in mind for future changes, though. This is an alternative solution to the one in 53dee8a (currently on the `screenshot-with-offscreen-rendering` branch). That method (using Electron's new "offscreen" rendering mode) is *much* faster than this and vastly simplifies the code, but has more ways it can fail.
In an extreme failure scenario, this will help to make sure Nightmare really can't hang just waiting for a new frame to render (this generally shouldn't ever happen given the previous commit's change to use the debugger to force new frames, but better safe than sorry here). After a long enough delay, it's probably OK to pull from the backing store anyway. In any event, an inaccurate screenshot is *probably* a better worst-case scenario than a hang.
|
Note the offscreen rendering branch has window sizing issues as a result of an Electron bug (electron/electron#8224), so it’s probably just untenable for now. |
|
One question: if you're using the remote debugging protocol, doesn't that mean you can't use the debugger (or have the development tray) open at the same time? If I'm not mistaken, this is the same issue that It's also entirely possible this got fixed in a more recent version of Electron. I guess what I'm beating around the bush and saying that either this needs to be tested for and/or should be included in the readme. Otherwise, I'm +1 on this as it solves more problems than it creates, imho. Thoughts? |
If we leave the remote debugger attached, users won't be able to open the local debugger in a visible window for manual testing after a screenshot has been taken (or rather, they can open it, but it won't work). To accomodate manual debugger usage, immediately detach once our work is done.
😲 I was entirely unaware of that as an issue and hadn’t tested it. The good news is that if a user has opened the local debugger before screenshotting, screenshots still work (see the Since you’ve seen this issue before, it’d be great if you give it a go as well, @rosshinkley. |
No worries. I just knew this was a stumbling block in the past, thought I'd bring it up.
I re-read what you had done and tinkered for a bit, I'm going to go ahead and say LGTM and that this should get included. Thanks! |
|
Sweet! Glad this can improve things a little. |
Since the initial implementation of
FrameManager, Electron has added an API for Chromium's remote debugging protocol, which can visually highlight a portion of the page. This change uses that functionality to force a new frame to be rendered, which is much more reliable than the way Nightmare currently tries to force new frames to render by fiddling around with the DOM (see people experiencing problems with this in #555, #736, #809). It also has the benefit of not doing anything page content can observe, ensuring that any JS or CSS won't modify the page in response to Nightmare's attempt to take a screenshot.This also adds a timeout. We should never hit it when using the debugger, but I figure it’s better to be safe than sorry here, especially after the hangs people have been experiencing.
In future versions of the remote debugging protocol, it will be possible to directly capture an image of the page, but that feature is still experimental (so it could be removed) and Electron does not yet support it anyhow. Something to keep in mind for future changes, though.
This is an alternative solution to the one in 53dee8a (currently on the
screenshot-with-offscreen-renderingbranch). That method (using Electron's new "offscreen" rendering mode) is much faster than this and vastly simplifies the code, but has more ways it can fail and has other side effects. If that approach looks better, I can submit it as a PR. We can also look into combining the two if the side effects are acceptable.